[HttpKernel] InlineFragmentRenderer should add the X-Forwarded-For of real client IP #7557
Conversation
InlineFragmentRenderer should add the X-Forwarded-For of real client IP, just like EsiFragmentRenderer expects. To this work, the getClientIp was updated with a fix for that.
…t of [X-Forwarded-For: client, proxy1, proxy2]
|
Build is broken because scrutinizer is reporting a previous warning from PHP Code Sniffer (Each class must be in a file by itself), which already existed before this PR. |
| @@ -696,6 +696,21 @@ public function getClientIp() | |||
| $clientIps[] = $ip; | |||
|
|
|||
| $trustedProxies = self::$trustProxy && !self::$trustedProxies ? array($ip) : self::$trustedProxies; | |||
|
|
|||
| //If is there any forward_for IP address, this is the real client IP address | |||
| if ($this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])) { | |||
There was a problem hiding this comment.
At this point the condition inside if always evaluates to true, so the code following this block will never be reached. Anyway this block just duplicates the original logic.
There was a problem hiding this comment.
Moreover, it brings back the vulnerability fixed by introducing the list of trusted proxies, since it bypasses the check.
There was a problem hiding this comment.
$this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])
This just work if the X-Forwarded-For exists...
This dont duplicates the original logic, it just get the client ip address from the X-Forwarded-For header (client, proxy1, proxy2, ...) instead of the hardcoded 127.0.0.1... this was made to avoid this hardcoded IP address
It does not bypass the check... there is no check for this, once a time that the real client IP address is not at the trustedProxies.
There was a problem hiding this comment.
@kinncj Getting it from the X-Forwarded-For when trusting proxies is already handled by the existing code. The returned ip should always be the closest untrusted ip, not the original IP (as headers can be forged by the client)
There was a problem hiding this comment.
X-Forwarded-For should return always the original IP.
The general format of the field is:
X-Forwarded-For: client, proxy1, proxy2
where the value is a comma+space separated list of IP addresses, the left-most being the original client, and each successive proxy that passed the request adding the IP address where it received the request from. In this example, the request passed through proxy1, proxy2, and then proxy3 (not shown in the header). proxy3 appears as remote address of the request.
If this is the validation it must be wrong...
This validation is breaking the original X-Forwarded-For semantic...
The validation must be a new issue and not related to this PR... you cannot avoid the validation removing the original IP address.
There was a problem hiding this comment.
@kinncj As mentioned by @stof, the X-Forwarded-For header may be forged by client, so you should not trust its contents. But if you are sure you need this, you can always get the header contents by yourself. Or you could suggest adding something like Request::getUntrustedClientIp(), but you should not introduce security holes in the name of your convenience :)
There was a problem hiding this comment.
Talking with blanco, we found an another sollution.
check: #7559
There was a problem hiding this comment.
And FYI, anything can be forged by the client... even a X-Parmegiana-For... you cannot act as a firewall/proxy for this...
Anyway,
rails/rails#2490
There was a problem hiding this comment.
And again, the InlineFragmentRenderer is not a real request, so it cannot overwrite the original REMOTE_ADDR to 127.0.0.1
RFC 2616, Sec 5.
|
I'm closing this issue because we did not reach a common opinion. |
This PR was squashed before being merged into the master branch (closes #7559). Discussion ---------- [HttpFoundation] [HttpKernel] Internal sub-requests should have X-Forwarded-For header providing real client IP This is a better alternative to fix issue highlighted in #7554 and #7557. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7554, #7557 | License | MIT When dealing with inline fragment renderer, it emulates an internal request by overriding the REMOTE_ADDR on Request. This is true, since conceptually request came from local server. The problem that this introduces is that overriding the server value, it turns into an impossible state to retrieve the real client ip, only returning the local server IP (which is hardcoded to 127.0.0.1). This patch takes the same approach as a Varnish call (it behaves the exact same way, reusing all code built for handling client ip handling on sub-requests), populating the X-Forwarded-For header and also making getClientIp smarter by removing possible local IP addresses from being considered as the client IP address. Commits ------- 773e109 [HttpFoundation] [HttpKernel] Internal sub-requests should have X-Forwarded-For header providing real client IP
InlineFragmentRenderer should add the X-Forwarded-For of real client IP, just like EsiFragmentRenderer expects.
To this work, the getClientIp was updated with a fix for that.